Remove references to aggregation attribute on Workflows #4339
Conversation
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| @@ -309,7 +311,7 @@ | |||
| end | |||
|
|
|||
| it 'updates the content model' do | |||
| expect{ resource.reload }.to change{ resource.strings } | |||
| expect{ resource.reload }.to change(resource, :strings) | |||
There was a problem hiding this comment.
Layout/SpaceBeforeBlockBraces: Space missing to the left of {.
|
Seems most sensible to just deprecate the AggregationsController now in advance of #4303. This PR should fix the issues causing those specs to fail, and that PR fully refactors the AggregationsController and its spec. |
| @@ -87,7 +87,7 @@ | |||
| expect(resolved_scope).to include(public_aggregation) | |||
| end | |||
|
|
|||
| it 'includes aggregations from owned private projects' do | |||
| xit 'includes aggregations from owned private projects' do | |||
There was a problem hiding this comment.
what are the plans with skipped tests? Mainly asking because I can see this easily being forgotten in history as skipped and never addressed again.
If the plans are to delete these tests, maybe we can do that now? Or maybe the batch aggregation PR/ a follow up PR will take care of these skipped tests removals/give these specs more context so that we can un-skip the tests.
Maybe adding a comment or something before the skipped tests to explain what the plan is would be helpful. BUT if this is quickly mitigated by the followup PR, them we can leave as is.
There was a problem hiding this comment.
Everything related to aggregations is being refactored by the batch aggregation PR. These bits also had to be skipped here because the notion of a private project was handled by the attribute that this removes. I'll remove them if you prefer, mostly just commented for clarity here and either way it's all getting removed when that PR merges (hopefully very soon!).
The workflows table has an
aggregationattribute that's going to collide with the new relation to the aggregations table. As far as I can tell, the only use for this was to indicate whether associated aggregation resources were "public" or "private".This PR includes
aggregationin the Workflow model'signored_columnsand removes references to this column from elsewhere. A subsequent PR will include the migration to remove the column from the database. Because ActiveRecord caches attributes at runtime (andaggregationis serialized on workflow resources), this will prevent exceptions between deployment and migration. I also needed to do some short circuiting in the policy specs, but they're all getting removed/rewritten in the batch agg PR.Mostly this is to satisfy strong_migrations and stick to best practices. The migration PR will be a quick one to review/deploy before the batch agg PR merges.
Review checklist
apiary.apibfile?